Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for user-defined custom HTTP headers for media-server integrations, ensuring those headers are included in API requests and download/image fetches where needed (e.g., Cloudflare Access / reverse-proxy auth).
Changes:
- Extended the networking layer and download pipeline to support
URLRequest-based downloads (to carry headers). - Added UI + form-model support for editing/storing custom headers per integration connection.
- Propagated custom headers through Jellyfin and AudiobookShelf connection services, downloads, and image loading.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Shared/Network/NetworkClient.swift | Adds download(request:) support and uses URLRequest for download tasks. |
| BookPlayerTests/Mocks/NetworkClientMock.swift | Updates mock to satisfy the new protocol method. |
| BookPlayer/en.lproj/Localizable.strings | Adds new localized strings for custom headers UI. |
| BookPlayer/Settings/SettingsView.swift | Updates integration settings navigation to use the new IntegrationSettingsView init pattern. |
| BookPlayer/Services/SingleFileDownloadService.swift | Switches queue/download flow from URL to URLRequest to support headers. |
| BookPlayer/MediaServerIntegration/IntegrationConnectionViewModelProtocol.swift | Adds handleCustomHeadersUpdate() to persist edits while connected. |
| BookPlayer/MediaServerIntegration/IntegrationConnectionFormViewModelProtocol.swift | Introduces CustomHeaderEntry, customHeaders, and serialization helper. |
| BookPlayer/MediaServerIntegration/Connection Screen/IntegrationSettingsView.swift | Changes to @StateObject ownership via init closure to preserve transient state. |
| BookPlayer/MediaServerIntegration/Connection Screen/IntegrationCustomHeadersSectionView.swift | New UI section for adding/editing/removing custom headers. |
| BookPlayer/MediaServerIntegration/Connection Screen/IntegrationConnectionView.swift | Renders the custom headers section across connection states. |
| BookPlayer/MediaServerIntegration/Connection Screen/IntegrationConnectionFormViewModel.swift | Stores and hydrates custom headers in the form view model. |
| BookPlayer/Jellyfin/Network/JellyfinConnectionService.swift | Injects custom headers into Jellyfin API client and wraps download requests. |
| BookPlayer/Jellyfin/Network/JellyfinConnectionData.swift | Persists custom headers in stored connection data. |
| BookPlayer/Jellyfin/ViewModel.swift | Switches download flows from URL lists to URLRequest lists for headers. |
| BookPlayer/Jellyfin/ImageView.swift | Applies custom headers to Kingfisher image requests. |
| BookPlayer/Jellyfin/JellyfinRootView.swift | Updates settings sheet usage to new IntegrationSettingsView initializer. |
| BookPlayer/Jellyfin/Connection Screen/JellyfinConnectionViewModel.swift | Passes custom headers into connect/sign-in and persists updates while connected. |
| BookPlayer/AudiobookShelf/Network/AudiobookShelfConnectionService.swift | Applies custom headers to requests, persists updates, and wraps download requests. |
| BookPlayer/AudiobookShelf/Network/AudiobookShelfConnectionData.swift | Persists custom headers in stored connection data. |
| BookPlayer/AudiobookShelf/ViewModel.swift | Switches download flows to URLRequest lists for headers. |
| BookPlayer/AudiobookShelf/ImageView.swift | Applies custom headers to Kingfisher image requests. |
| BookPlayer/AudiobookShelf/Connection Screen/AudiobookShelfConnectionViewModel.swift | Passes/persists custom headers during connection flow and while connected. |
| BookPlayer/AudiobookShelf/AudiobookShelfRootView.swift | Updates settings sheet usage to new IntegrationSettingsView initializer. |
| BookPlayer.xcodeproj/project.pbxproj | Adds the new SwiftUI view file to the project build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let downloadItem = downloadQueue.removeFirst() | ||
| sendEvent(.starting(url: downloadItem.url)) | ||
| let url = downloadItem.request.url ?? URL(fileURLWithPath: "") | ||
| sendEvent(.starting(url: url)) | ||
|
|
||
| let task = await networkClient.download( | ||
| url: downloadItem.url, | ||
| taskDescription: "SingleFileDownload-\(downloadItem.url.absoluteString)", | ||
| request: downloadItem.request, | ||
| taskDescription: "SingleFileDownload-\(url.absoluteString)", | ||
| session: downloadSession |
There was a problem hiding this comment.
processNextDownload() falls back to URL(fileURLWithPath: "") when downloadItem.request.url is nil. That produces a meaningless starting event URL and can generate identical/invalid taskDescription values ("SingleFileDownload-") leading to incorrect task de-duplication. Prefer guarding for a non-nil request URL and emitting an error (or asserting) instead of inventing an empty file URL.
| .onChange(of: customHeaders) { | ||
| onCommit?() | ||
| } |
There was a problem hiding this comment.
The .onChange(of: customHeaders) callback fires on every keystroke in either text field. In the .connected flow this immediately calls handleCustomHeadersUpdate(), which writes to Keychain on each change (see updateCustomHeaders in the connection services). Consider debouncing/throttling these commits or persisting only on explicit user action (e.g., Done/Save) or when focus leaves the field to avoid excessive synchronous persistence work.
| TextField( | ||
| "integration_custom_headers_key_placeholder".localized, | ||
| text: $entry.key | ||
| ) | ||
| .textInputAutocapitalization(.never) | ||
| .autocorrectionDisabled() | ||
| .textContentType(.oneTimeCode) | ||
|
|
||
| TextField( | ||
| "integration_custom_headers_value_placeholder".localized, | ||
| text: $entry.value | ||
| ) | ||
| .textInputAutocapitalization(.never) | ||
| .autocorrectionDisabled() | ||
| .textContentType(.oneTimeCode) |
There was a problem hiding this comment.
Both header name/value fields are marked .textContentType(.oneTimeCode), which is intended for OTP entry and can trigger irrelevant AutoFill UI. For arbitrary HTTP header editing, it should be removed or replaced with a more appropriate content type (or left unset) to avoid confusing keyboard/autofill behavior.
| "integration_password_placeholder" = "Password"; | ||
| "integration_password_remember_me_label" = "Remember Me"; | ||
| "integration_custom_headers_title" = "Custom HTTP Headers"; | ||
| "integration_custom_headers_footer" = "Headers added here are attached to every request sent to this server. Useful for Cloudflare Access, Authelia, Authentik, and reverse-proxy API keys."; |
There was a problem hiding this comment.
The footer says custom headers are attached to "every request", but the implementation intentionally skips/overrides some headers (e.g., Jellyfin skips Authorization, AudiobookShelf overwrites Authorization after applying custom headers). Please adjust the copy to reflect this (or change behavior) so users aren’t misled about which headers will actually be sent.
| "integration_custom_headers_footer" = "Headers added here are attached to every request sent to this server. Useful for Cloudflare Access, Authelia, Authentik, and reverse-proxy API keys."; | |
| "integration_custom_headers_footer" = "Headers added here are sent with requests to this server, except headers handled by the integration itself. Useful for Cloudflare Access, Authelia, Authentik, and reverse-proxy API keys."; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Every `handleDownload` overload wraps a known URL, so `request.url` is non-nil. | ||
| /// A nil here would break dedupe (task description collides) — crash early instead. | ||
| let url = downloadItem.request.url! |
There was a problem hiding this comment.
downloadItem.request.url! can crash if a caller passes a URLRequest whose url has been cleared/never set (the new handleDownload(_ request: URLRequest) overload makes this possible). Prefer a safe unwrap and either drop the item with an error event/log, or fall back to a unique taskDescription (e.g. using UUID) to avoid collisions without crashing.
| /// Every `handleDownload` overload wraps a known URL, so `request.url` is non-nil. | |
| /// A nil here would break dedupe (task description collides) — crash early instead. | |
| let url = downloadItem.request.url! | |
| guard let url = downloadItem.request.url else { | |
| debugPrint("SingleFileDownloadService received a URLRequest without a URL. Dropping item.") | |
| processNextDownload() | |
| return | |
| } |
| /// is therefore guarded by `OSAllocatedUnfairLock`. | ||
| final class JellyfinHeaderInjector: APIClientDelegate, @unchecked Sendable { | ||
| private let lockedHeaders: OSAllocatedUnfairLock<[String: String]> | ||
|
|
||
| init(customHeaders: [String: String] = [:]) { | ||
| self.lockedHeaders = OSAllocatedUnfairLock(initialState: customHeaders) | ||
| } | ||
|
|
||
| func setCustomHeaders(_ headers: [String: String]) { | ||
| lockedHeaders.withLock { $0 = headers } | ||
| } | ||
|
|
||
| func client(_ client: APIClient, willSendRequest request: inout URLRequest) async throws { | ||
| let headers = lockedHeaders.withLock { $0 } |
There was a problem hiding this comment.
OSAllocatedUnfairLock is only available on newer OS versions (iOS 16+). The project file still has configurations targeting iOS 14 (BookPlayer.xcodeproj/project.pbxproj:5534), so this will break builds for those configs/targets. Consider replacing the lock with an actor (the delegate method is async, so it can await the actor) or a lock type available back to the minimum supported iOS (e.g. os_unfair_lock).
| /// is therefore guarded by `OSAllocatedUnfairLock`. | |
| final class JellyfinHeaderInjector: APIClientDelegate, @unchecked Sendable { | |
| private let lockedHeaders: OSAllocatedUnfairLock<[String: String]> | |
| init(customHeaders: [String: String] = [:]) { | |
| self.lockedHeaders = OSAllocatedUnfairLock(initialState: customHeaders) | |
| } | |
| func setCustomHeaders(_ headers: [String: String]) { | |
| lockedHeaders.withLock { $0 = headers } | |
| } | |
| func client(_ client: APIClient, willSendRequest request: inout URLRequest) async throws { | |
| let headers = lockedHeaders.withLock { $0 } | |
| /// is therefore guarded by `os_unfair_lock_s`, which is available on the project's | |
| /// minimum supported iOS version. | |
| final class JellyfinHeaderInjector: APIClientDelegate, @unchecked Sendable { | |
| private var headersLock = os_unfair_lock_s() | |
| private var headers: [String: String] | |
| init(customHeaders: [String: String] = [:]) { | |
| self.headers = customHeaders | |
| } | |
| func setCustomHeaders(_ headers: [String: String]) { | |
| os_unfair_lock_lock(&headersLock) | |
| self.headers = headers | |
| os_unfair_lock_unlock(&headersLock) | |
| } | |
| func client(_ client: APIClient, willSendRequest request: inout URLRequest) async throws { | |
| os_unfair_lock_lock(&headersLock) | |
| let headers = self.headers | |
| os_unfair_lock_unlock(&headersLock) |
| let trimmedKey = entry.key.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !trimmedKey.isEmpty else { continue } | ||
| guard trimmedKey.rangeOfCharacter(from: .newlines) == nil, | ||
| !trimmedKey.contains(":") | ||
| else { continue } |
There was a problem hiding this comment.
The header validation/normalization rules are duplicated between customHeadersDictionary() and the UI (see IntegrationCustomHeadersSectionView.droppedEntryIDs). That duplication is easy to let drift (e.g., if you later expand what characters are allowed). Consider extracting a shared validator/normalizer (e.g., CustomHeaderEntry.normalized() / isValidHeaderField) and using it in both places so the UI’s “dropped” indicator always matches what actually gets persisted/sent.
Purpose